-
Notifications
You must be signed in to change notification settings - Fork 345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Capability adds for ParmParse enum #4119
Conversation
I got clang-tidy complaints about ParmParse By the way, I really like the added capability for parmparsing enums. I can already think of a ton of places where that can drastically simplify code in various apps. |
Re: constness, one could argue that Yes, we should add https://isocpp.org/wiki/faq/const-correctness#logical-vs-physical-const |
1881429
to
9e70570
Compare
Agreed that add/addarr should intuitively not be const despite what clang-tidy thinks. I added some NOLINTs for those functions instead of making const. |
Src/Base/AMReX_ParmParse.H
Outdated
if (exist) { | ||
try { | ||
ref = amrex::getEnum<T>(s); | ||
} catch (...) { | ||
amrex::Print() << "amrex::ParmParse::query (input name: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to allow the users to turn off warning messages. So we either remove this, or put this inside if (amrex::Verbose() > xxx)
, or add the message to what is being thrown by amrex::getEnum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Summary
const
prefixedName
function is made a public member instead of a protected member.Additional background
The use case for these changes is erf-model/ERF#1772
Checklist
The proposed changes: